-
-
Notifications
You must be signed in to change notification settings - Fork 14.4k
Introduce #[diagnostic::on_move(message)] #150935
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Introduce #[diagnostic::on_move(message)] #150935
Conversation
|
These commits modify the If this was unintentional then you should revert the changes before this PR is merged. Some changes occurred in compiler/rustc_passes/src/check_attr.rs |
|
rustbot has assigned @JonathanBrouwer. Use |
|
r? @estebank |
|
I'm currently working on migrating the existing diagnostic attribute infrastructure, can you do this PR after that? It's going to be quite a mess to resolve that conflict. Is this meant to be only used by the standard library or also by users? If only by std, I think you should forego the attribute and instead check whether T implements |
|
Yeah I can completly wait until you finished your work and do my PR after that, It was my intention to be honest . Resolving conflicts or rebasing is not an issue for me. |
9a0162d to
8071a7d
Compare
|
Some changes occurred in compiler/rustc_attr_parsing cc @jdonszelmann, @JonathanBrouwer Changes to the size of AST and/or HIR nodes. cc @nnethercote Some changes occurred in compiler/rustc_hir/src/attrs |
This comment has been minimized.
This comment has been minimized.
|
I have reworked my code and moved everything to
|
8071a7d to
09506a4
Compare
This comment has been minimized.
This comment has been minimized.
09506a4 to
829651a
Compare
This comment has been minimized.
This comment has been minimized.
|
Rebased onto main, I have also fixed the size of |
| #[diagnostic::on_move( | ||
| message = Foo, | ||
| //~^ERROR expected a literal (`1u8`, `1.0f32`, `"string"`, etc.) here, found expression | ||
| label = "Bar", | ||
| )] | ||
| #[diagnostic::on_move( | ||
| message = "Foo" | ||
| label = "Bar", | ||
| //~^ERROR expected `,`, found `label` | ||
| )] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can't be an error; this must be a lint. But you will need #151516 to do so.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi. Since this morning, these "cases" are no longer handled at all. The parser does not report anything and I get an empty MetaItemListParser (there are no sub_parsers) in onMoveParser::convert , see #t-compiler/help > #diagnostic::on_move(message) (PR #150935) @ 💬 for the full description. Is it expected ? I mean I cannot emit the lint myself, I have an empty ArgParser context, and the parser reports nothing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think, I got it. This is due to https://github.com/rust-lang/rust/blob/main/compiler/rustc_attr_parsing/src/parser.rs#L132 . I can of course emit a lint myself when the MetaItemListParser is empty, it's a shame to no longer have the context of the error, we had a more fine grained context with a span before. The span now will be the full diagnostic attribute.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is possible to replace the entire span https://github.com/rust-lang/rust/blob/main/compiler/rustc_attr_parsing/src/parser.rs#L135 by e.span (or build a span from e) ? so we would have a span on the attr in this case instead of the whole diagnostic attribute.
| #[derive(Clone, Debug, Encodable, Decodable, HashStable_Generic, PrintAttribute)] | ||
| pub struct OnMoveAttrArg { | ||
| pub symbol: Symbol, | ||
| pub format_ranges: ThinVec<(usize, usize)>, | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I plan on moving a fair bit of the format and string stuff that on_unimplemented uses into rustc_hir; you should be able to reuse some of that in this PR :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I will do
This might be helpful for smart pointers to explains why they aren't Copy and what to do instead or just to let the user know that .clone() is very cheap and can be called without a performance penalty.
829651a to
b12aa8b
Compare
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
|
Rebased onto main. I have covered changes related to #151516 .
|
cc #149862
This is a first proposal. I have deliberately kept it simpler than
diagnostic::on_unimplemented.Few questions/remarks:
rustc_astfrom the borrowck ?Suggestions are welcomed !